Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: added documentation to enable Slack notification post deployment task (#787) #788

Merged

Conversation

sudiptob2
Copy link
Member

@sudiptob2 sudiptob2 commented Feb 7, 2023

Fixes #787

  • Removed postDeploymentTasks from the base/app.yaml
  • Added documentation to configure and enable postDeploymentTasks
  • Added implement-slack-notification task in the docs/task page

@thschue
Copy link
Contributor

thschue commented Feb 8, 2023

Hello, and thank you for your contribution! I think it's a good example to show the slack notifications in the example, but we should keep in mind that not everyone might have slack which could lead to problems when trying out the lifecycle toolkit.

Would you mind if you write some documentation (Tasks) on how to enable Slack notifications instead of adding this to the core of the examples?

@sudiptob2
Copy link
Member Author

how to enable Slack notifications instead of adding this to the core

@thschue Yes sure. I will update PR. Thanks for reviewing the PR

@sudiptob2 sudiptob2 changed the title fix: added slack notification post deployment task (#787) fix: added documentation to eable slack notification post deployment task (#787) Feb 8, 2023
@sudiptob2
Copy link
Member Author

@thschue updated according to your suggestion, could you please check the PR. Thanks much for your time and support.

@thschue
Copy link
Contributor

thschue commented Feb 8, 2023

Would you like to move this to the docs (docs/tasks/...) ?

@sudiptob2
Copy link
Member Author

Would you like to move this to the docs (docs/tasks/...) ?

Thanks, working on it.

@sudiptob2
Copy link
Member Author

@thschue, Should I copy the entire readme under the docs/taks. We can name the task as Deploying-sample-app.

@sudiptob2
Copy link
Member Author

@thschue I think creating a new issue for adding a "Deploying sample app" section to the docs/task will be better. I'll write a guide for deploying and testing the sample app which can be added to the documentation site. Let me know your thoughts."

@StackScribe
Copy link
Contributor

The current structure of the documents has some issues and these will be resolved in time. The section called "Tasks" is not well-structured -- we need to have a section called "Installation". Then I think we will have a "User Guide" or some such that discusses how to implement various features/capabilities into ones Keptn Application. In that User Guide, I envision a section about KeptnTaskDefinition that begins with a page like what is in the "Write Keptn Tasks" page, and then has other pages about specific features/functionality. One of these could be "Implementing a post-deployment Slack notification" and I suggest that you use this text to create that page. Although maybe it should be called just "Implement Slack notifications" because I think one would use the same method to implement Slack notifications in the pre-deployment phase and perhaps elsewhere.

For now, give that file a weight that puts it after the write-tasks file but before evaluate. If you don't know what I mean by "weight", check the contents of the CONTRIBUTING.md file for the docs that is currently in the PR I sent you. It's part of the metadata for each file. Many doc engines specify the files to be built and the order by using some sort of a list. Hugo instead builds everything that is there (unless it is explicitly excluded) and uses the "weight" parameter to control the order in which the files are displayed.

@sudiptob2 sudiptob2 force-pushed the fix/787/add-mising-post-deployment-task branch from e17f58a to 3e93310 Compare February 8, 2023 12:37
@sudiptob2
Copy link
Member Author

Hi @StackScribe, I am moving forward with your suggestion. Would appreciate if you could have a look at the PR.

@sudiptob2 sudiptob2 requested a review from StackScribe February 8, 2023 12:39
thschue
thschue previously approved these changes Feb 8, 2023
Copy link
Contributor

@thschue thschue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Merging #788 (95035f0) into main (b5c5801) will decrease coverage by 0.06%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #788      +/-   ##
==========================================
- Coverage   57.75%   57.69%   -0.06%     
==========================================
  Files          91       91              
  Lines        7243     7243              
==========================================
- Hits         4183     4179       -4     
- Misses       2891     2894       +3     
- Partials      169      170       +1     
Impacted Files Coverage Δ
...lers/lifecycle/keptnworkloadinstance/controller.go 80.99% <0.00%> (-1.81%) ⬇️
Flag Coverage Δ
component-tests 47.77% <ø> (-0.63%) ⬇️
keptn-lifecycle-operator 53.60% <ø> (ø)
klt-cert-manager 67.50% <ø> (ø)
scheduler 21.17% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sudiptob2 sudiptob2 requested review from thschue and removed request for StackScribe February 9, 2023 02:43
@sudiptob2 sudiptob2 requested review from StackScribe and removed request for thschue February 9, 2023 02:43
Copy link
Contributor

@thschue thschue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mowies mowies changed the title fix: added documentation to eable slack notification post deployment task (#787) docs: added documentation to enable Slack notification post deployment task (#787) Feb 11, 2023
Copy link
Member

@grabnerandi grabnerandi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@thisthat
Copy link
Member

Thanks for the contribution @sudiptob2
The pipeline fails but has nothing to do with these changes :)

@thisthat thisthat merged commit 28a7319 into keptn:main Feb 13, 2023
@keptn-bot keptn-bot mentioned this pull request Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

postDeploymentTasks task is missing in the sample-application example
5 participants